Add hasFields, Keys, Pluck, Values and Without manipulations #52 #69

Th3Mouk · 2018-08-07T16:38:57Z

Purpose

Open Questions and Pre-Merge TODOs

Add tests suite

Solution

Theses operators aren't tested yet

Th3Mouk · 2018-08-08T10:07:30Z

@tbolier I'm totally stuck on operators implementation, i don't get the global logic of the implementation. Especially on the hasFields one.
The try is about:

r.db('test')
  .table('tabletest')
  .filter(r.row.hasFields('number'))

And the implementation is in HasFieldsTest

The problems are:

row() fonction must be without parameters (i think it's cool now)
filter() operator is not well documented

I don't understand if the HasFields operators must return a Row, a QueryInterface, himself, or something else.
If I must create a ManipulationTrait or use the Row classes to add the method. And why here in FilterByRow my manipulation query (HasFields object) is null.

I'm afraid theses problem impact the date operators implementation too. I need to start the dev of a new API and without them I probably will switch to another driver.

tbolier

Great work, will you add the missing unit tests or do you need help with that?

tbolier · 2018-08-20T10:14:30Z

src/Query/Manipulation/Keys.php

+    /**
+     * @var array
+     */
+    private $keys;


Can you remove this unused property?

Th3Mouk · 2018-08-21T15:01:49Z

Yeah please I need some explanations to finish those operators

tbolier

@Th3Mouk I will code along in this pull request to help if that is okay with you?

I have some comments ready in the pull request review, but I think participating makes it a little easier.

For the filter implementation, check the current GetFieldLogic as example. What are your the issues you are running into at the moment?
The row parameter can be optional, that's no problem. I see you already worked that out.
The return types can differ per implementation, so the return type for row()->hasFields() can be different than get('ID').hasFields(). Traits are only there to make code reusable, if we cannot reuse it, because the logic is different, there is no issue in duplicating some code / even traits. Of course we try to keep it as clean, as less complex, as less code duplication as possible, but this should not block our ease of implementation. Choose for the easy route via duplication, and later we can always slim it down again, if we see we have blocks of duplicate codes.

I will clone your branch and get the coding in this week.

tbolier · 2018-08-27T15:14:46Z

src/Query/Builder.php

@@ -70,7 +70,7 @@ public function ordening(string $key): Ordening
        return $this->ordering;
    }

-    public function row(string $value): Row
+    public function row(?string $value = null): Row


Isn't the ? is obsolete since null probably will never be passed?

Th3Mouk · 2018-08-28T08:46:34Z

No problem ! I prefer a few exemples to long explanations

tbolier · 2018-09-08T19:19:41Z

Hey @Th3Mouk

I've fixed the hasFields manipulation command for Table and Row by creating two objects. The different between a Table query with hasFields is that the table query should be nested in the hasFields array, like you had implemented initially. For the Row implementation that query should never be nested, it's just a command that gets nested within the FilterByRow command, since it's passed as parameter there. Very confusing, but please look at my commit added to this PR:

212b333

This is what I meant with code duplication is fine, if the implementation differs. We do not need to put anything in traits if we have different implementations.

Does this make sense to you?

tbolier · 2018-09-08T19:37:29Z

@Th3Mouk For Keys, Pluck, Values and Without the implementation was fine, only a nesting query fix and returning concrete types for auto completion at client.

tbolier · 2018-09-08T20:13:30Z

@Th3Mouk I am planning to merge this, are you happy with the outcome and feedback, and do you agree I will merge this?

tbolier · 2018-09-08T20:14:38Z

A good tip to debug is to check the XHR request headers from the admin rethinkDB panel, this will show you the raw JSON query send to RethinkDB, I usually compare this to the PHP raw query that I will copy paste from xDebug.

Th3Mouk added this to the Version 2.0 milestone Aug 7, 2018

Th3Mouk self-assigned this Aug 7, 2018

Th3Mouk removed this from the Version 2.0 milestone Aug 7, 2018

tbolier requested changes Aug 20, 2018

View reviewed changes

src/Query/Manipulation/Keys.php Outdated

/**

* @var array

*/

private $keys;

Copy link

Owner

tbolier Aug 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this unused property?

tbolier requested changes Aug 27, 2018

View reviewed changes

Th3Mouk and others added 7 commits September 8, 2018 21:16

Add some documents manipulation operators

310d032

Row must be nullable to operate on all the document

2aa63ea

WIP HasFields

855a79b

Remove unused property

f9e67cb

WIP Tests

e692bfa

Move GetFieldLogic to Query/Manipulation namespace

63f20a8

Fix hasFields implementation for Table and Row

212b333

tbolier force-pushed the feature/doc-manipulation branch from 485bc41 to 212b333 Compare September 8, 2018 19:16

Fix keys implementation manipulation

81d6f52

tbolier changed the title ~~[WIP] Add some documents manipulation operators~~ Add hasFields, Keys, Pluck, Values and Without manipulations Sep 8, 2018

tbolier added 2 commits September 8, 2018 21:46

Fix pluck implementation manipulation

f568cf9

Fix values implementation manipulation

08d4b83

tbolier changed the title ~~Add hasFields, Keys, Pluck, Values and Without manipulations~~ Add hasFields, Keys, Pluck, Values and Without manipulations (#52) Sep 8, 2018

Fix without implementation manipulation

0d2dcd3

tbolier force-pushed the feature/doc-manipulation branch from 94f2651 to 0d2dcd3 Compare September 8, 2018 20:01

Remove obsolete nullable type, since if not passed it defaults to null

c05af96

tbolier changed the title ~~Add hasFields, Keys, Pluck, Values and Without manipulations (#52)~~ Add hasFields, Keys, Pluck, Values and Without manipulations #52 Sep 8, 2018

tbolier approved these changes Sep 8, 2018

View reviewed changes

tbolier requested review from AxaliaN and marcgeurts September 8, 2018 20:20

tbolier merged commit d3fd031 into master Sep 9, 2018

tbolier deleted the feature/doc-manipulation branch September 9, 2018 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hasFields, Keys, Pluck, Values and Without manipulations #52 #69

Add hasFields, Keys, Pluck, Values and Without manipulations #52 #69

Th3Mouk commented Aug 7, 2018 •

edited by tbolier

Loading

Th3Mouk commented Aug 8, 2018

tbolier left a comment

tbolier Aug 20, 2018

Th3Mouk commented Aug 21, 2018

tbolier left a comment

tbolier Aug 27, 2018

Th3Mouk commented Aug 28, 2018

tbolier commented Sep 8, 2018 •

edited

Loading

tbolier commented Sep 8, 2018 •

edited

Loading

tbolier commented Sep 8, 2018

tbolier commented Sep 8, 2018

Add hasFields, Keys, Pluck, Values and Without manipulations #52 #69

Add hasFields, Keys, Pluck, Values and Without manipulations #52 #69

Conversation

Th3Mouk commented Aug 7, 2018 • edited by tbolier Loading

Purpose

Open Questions and Pre-Merge TODOs

Solution

Th3Mouk commented Aug 8, 2018

tbolier left a comment

Choose a reason for hiding this comment

tbolier Aug 20, 2018

Choose a reason for hiding this comment

Th3Mouk commented Aug 21, 2018

tbolier left a comment

Choose a reason for hiding this comment

tbolier Aug 27, 2018

Choose a reason for hiding this comment

Th3Mouk commented Aug 28, 2018

tbolier commented Sep 8, 2018 • edited Loading

tbolier commented Sep 8, 2018 • edited Loading

tbolier commented Sep 8, 2018

tbolier commented Sep 8, 2018

Th3Mouk commented Aug 7, 2018 •

edited by tbolier

Loading

tbolier commented Sep 8, 2018 •

edited

Loading

tbolier commented Sep 8, 2018 •

edited

Loading